Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactoring: display driver in r_raster.c fixing duplicateExpression warning #3357

Merged
merged 2 commits into from
Jan 22, 2024
Merged

refactoring: display driver in r_raster.c fixing duplicateExpression warning #3357

merged 2 commits into from
Jan 22, 2024

Conversation

Sharansrj567
Copy link
Contributor

@Sharansrj567 Sharansrj567 commented Jan 12, 2024 •
edited
Loading

This pull request addresses a duplicateExpression warning identified by cppcheck in the lib/display/r_raster.c file related to the use of Cairo_Driver. The modifications aim to improve code clarity by refactoring redundant conditions in the Cairo_Driver logic.

Reasons:

  • Redundancy: The same function (Cairo_Driver()) is called in both the true and false branches of the conditional operator (? :), making the conditional check unnecessary.
  • Code Readability: The structure of the code might be confusing due to the extra conditional check.

Context:
These lines have not been touched for 16 years, and the conditional compilation includes or excludes Cairo_Driver() based on the availability of Cairo during GRASS build. If Cairo is available, it serves as the fallback driver; otherwise, the PNG driver is used.

Testing:
Manual testing has been performed to ensure that the behavior of the Cairo_Driver function remains unchanged. The modified code has undergone style checks using clang-format to maintain standard formatting.

@github-actions github-actions bot added C Related code is in C libraries docs display labels Jan 12, 2024
@echoix
Copy link
Member

echoix commented Jan 12, 2024

Unless other commits are hidden, blaming these lines show that it has been 16 years since these lines were touched.

For some GRASS reviewer:
Just to make sure about the current and future behavior:

  • Before: there is a conditional compilation directive to include or not Cairo_Driver() in a chain of ifs that choose a driver for the drv command. If GRASS is built with Cairo, Cairo is the fallback driver, or else it is the PNG driver.
  • After: the Cairo_Driver() is the fallback driver if GRASS is configured to build with Cairo, in the other case, we use PNG driver.

So that means that the PNG is always available when building, am I right? (There isn't any way to configure without png).

@neteler neteler added this to the 8.4.0 milestone Jan 14, 2024
@echoix echoix removed the docs label Jan 14, 2024
@nilason
Copy link
Contributor

nilason commented Jan 16, 2024

I believe the logic of the code is correct as it is. It depends on the value of the environmental variable GRASS_RENDER_IMMEDIATE (see Display drivers). It may be eg: default which leads to using Cairo if available. If Cairo is not available, it will default to PNG_Driver.

Cairo DISPLAY DRIVER:

The Cairo driver is used for GRASS display commands by default if available, otherwise PNG driver is used.

This case seems to me to be a false positive.

@nilason
Copy link
Contributor

nilason commented Jan 16, 2024

I believe the logic of the code is correct as it is. It depends on the value of the environmental variable GRASS_RENDER_IMMEDIATE (see Display drivers). It may be eg: default which leads to using Cairo if available. If Cairo is not available, it will default to PNG_Driver.

Cairo DISPLAY DRIVER:

The Cairo driver is used for GRASS display commands by default if available, otherwise PNG driver is used.

This case seems to me to be a false positive.

Scratch that. This is the end-result of this PR too. The only difference is that now it is clearer that the code should expect a value cairo.

@nilason nilason merged commit d5ed511 into OSGeo:main Jan 22, 2024
23 checks passed
@nilason
Copy link
Contributor

nilason commented Jan 22, 2024

@Sharansrj567 Thanks for your contribution!

@neteler neteler changed the title refactor: display driver in r_raster.c fixing duplicateExpression warning refactoring: display driver in r_raster.c fixing duplicateExpression warning Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilason nilason nilason approved these changes

Assignees

@nilason nilason

Labels
C Related code is in C display libraries
Projects
None yet
Milestone
8.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Sharansrj567 @echoix @nilason @neteler